Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement NoteMetadata protobuf message and NoteType enum #338

Merged
merged 14 commits into from
May 3, 2024

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented Apr 29, 2024

Closes #315
Closes #314

@igamigo igamigo marked this pull request as ready for review April 30, 2024 22:39
@igamigo igamigo requested review from polydez and hackaugusto May 2, 2024 23:40
Copy link
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good! I left a few comments inline. Most are minor - but three is one in which I wonder if using enum for note type was actually a good idea.

crates/proto/build.rs Outdated Show resolved Hide resolved
Comment on lines 8 to 12
enum NoteType {
PUBLIC = 0;
OFF_CHAIN = 1;
ENCRYPTED = 2;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these values be aligned with values here. If so, I wonder if there is a good way to keep them consistent (maybe using enum here was not such a good idea?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure about this. There cannot be protobuf enums that don't start with 0 so it's either creating a phantom variant, creating a custom mapping function (from protobuf's note type to base note type), or just removing it altogether and using a fixed32. For making sure they stay synced I can add a test that asserts this.

Copy link
Collaborator Author

@igamigo igamigo May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought it's not worth the added complexity if we can just convert the note type number. Removed in favor of a fixed32.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I came to the same conclusion. The only question - should we use int32 instead? (this way, encoding would take 1 byte instead of 4, but otherwise should be the same).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to uint32. Seemed similar in terms of encoding low values but marginally more correct for the use case.

crates/proto/src/domain/notes.rs Outdated Show resolved Hide resolved
crates/store/src/db/sql.rs Show resolved Hide resolved
crates/store/src/db/sql.rs Show resolved Hide resolved
crates/store/src/db/sql.rs Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! In addition to the small comment I left inline, I think we should change the type of note_type in protobuf message from fixed32 to int32.

crates/store/src/db/sql.rs Outdated Show resolved Hide resolved
@bobbinth bobbinth merged commit 410916c into next May 3, 2024
5 checks passed
@bobbinth bobbinth deleted the igamigo-proto-metadata branch May 3, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants